Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds PCP (Port Control Protocol) support: protocol primitives, a concurrent PCP client, a PCP-backed NAT adapter with discovery, PCP-first gateway discovery with fallback, and manager integration for periodic PCP health checks that can trigger mapping recreation; unit tests for the PCP client included. Changes
Sequence Diagram(s)sequenceDiagram
participant Mgr as PortForward Manager
participant HT as Health Ticker
participant RT as Renew Ticker
participant PCP as PCP Client
participant GW as PCP Gateway
HT->>Mgr: health tick
Mgr->>PCP: Announce(ctx)
PCP->>GW: ANNOUNCE request
GW-->>PCP: ANNOUNCE response (epoch)
PCP-->>Mgr: epoch, stateLost?
alt server restarted (stateLost)
Mgr->>PCP: AddPortMapping(ctx) (recreate)
PCP->>GW: MAP request
GW-->>PCP: MAP response (external port/IP)
PCP-->>Mgr: mapping info
Mgr->>RT: reset renew ticker
else no restart
Mgr->>Mgr: continue
end
RT->>Mgr: renew tick
Mgr->>PCP: AddPortMapping(ctx) (renew)
PCP->>GW: MAP request
GW-->>PCP: MAP response
PCP-->>Mgr: updated mapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@client/internal/portforward/pcp/client_test.go`:
- Around line 161-187: The test TestClientIntegration unconditionally calls
t.Skip; change it to read PCP_TEST_GATEWAY and PCP_TEST_LOCAL_IP from the
environment and only skip when those vars are not set (or are invalid). Parse
PCP_TEST_GATEWAY and PCP_TEST_LOCAL_IP into netip.Addr (or use
netip.MustParseAddr if you prefer panicking on invalid values), create the
client with NewClient(parsedGateway), call client.SetLocalIP(parsedLocalIP), and
then run the ANNOUNCE / AddPortMapping / DeletePortMapping logic as before;
ensure you use context.WithTimeout and return early with t.Skipf when env vars
are absent or parsing fails to allow manual runs without code edits.
In `@client/internal/portforward/pcp/client.go`:
- Around line 136-199: addPortMappingWithHint currently casts ports to uint16
without validating inputs; add explicit range checks before calling
buildMapRequest: ensure internalPort is within 1..65535 (reject and return an
error if out of range) and validate suggestedExtPort is within 0..65535 (or
1..65535 if you decide 0 should be invalid in your protocol), then only proceed
to call buildMapRequest and sendRequest; place these checks at the start of
addPortMappingWithHint (before the uint16 casts) and return clear fmt.Errorf
messages referencing internalPort/suggestedExtPort when invalid.
🧹 Nitpick comments (1)
client/internal/portforward/pcp/nat.go (1)
19-34: Track the IPv6 address-rotation TODO.Worth capturing as a follow-up issue or task so pinholes are refreshed if the local IPv6 changes.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
lixmal
force-pushed
the
nat-pmp-upnp
branch
2 times, most recently
from
9b45979 to
7fa7f1c
Compare
|
1 similar comment
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/portforward/pcp/client.go (1)
45-67: Consider returning an error for invalid gateway IPs.The constructors log invalid IPs at debug level but continue with a zero-value
netip.Addr. While callers likeDiscoverPCPvalidate the gateway beforehand, direct callers ofNewClientwith an invalid IP will silently fail later duringsendRequest. Consider returning an error or panicking to fail fast.♻️ Suggested defensive approach
func NewClient(gateway net.IP) *Client { addr, ok := netip.AddrFromSlice(gateway) if !ok { - log.Debugf("invalid gateway IP: %v", gateway) + log.Warnf("invalid gateway IP: %v", gateway) + return nil } return &Client{ gateway: addr.Unmap(), timeout: defaultTimeout, } }Alternatively, keep the current behavior if you prefer graceful degradation, but ensure callers handle the nil case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/portforward/pcp/client.go` around lines 45 - 67, NewClient and NewClientWithTimeout currently accept an invalid gateway IP silently (logging only) and return a Client with a zero-value netip.Addr which will fail later (e.g., in sendRequest); change the constructors to return (*Client, error) instead of *Client (update NewClient and NewClientWithTimeout signatures), validate netip.AddrFromSlice(gateway) and return a descriptive error when ok==false, and adjust callers (e.g., places that call DiscoverPCP or other direct callers) to handle the error; alternatively, if you prefer panic semantics, document and call panic on invalid gateway inside NewClient/NewClientWithTimeout so clients fail fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/internal/portforward/pcp/client.go`:
- Around line 45-67: NewClient and NewClientWithTimeout currently accept an
invalid gateway IP silently (logging only) and return a Client with a zero-value
netip.Addr which will fail later (e.g., in sendRequest); change the constructors
to return (*Client, error) instead of *Client (update NewClient and
NewClientWithTimeout signatures), validate netip.AddrFromSlice(gateway) and
return a descriptive error when ok==false, and adjust callers (e.g., places that
call DiscoverPCP or other direct callers) to handle the error; alternatively, if
you prefer panic semantics, document and call panic on invalid gateway inside
NewClient/NewClientWithTimeout so clients fail fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9139a537-e03a-449f-be91-853e11752ba1
📒 Files selected for processing (7)
client/internal/portforward/env.goclient/internal/portforward/manager.goclient/internal/portforward/pcp/client.goclient/internal/portforward/pcp/client_test.goclient/internal/portforward/pcp/nat.goclient/internal/portforward/pcp/protocol.goclient/internal/portforward/state.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/internal/portforward/state.go
- client/internal/portforward/pcp/client_test.go
|
2 participants
Describe your changes
This adds PCP (Port Control Protocol) support. PCP is discovered first and falls back to NAT-PMP/UPnP if unavailable.
PCP's advantages over NAT-PMP:
Key features:
New environment variables:
NB_DISABLE_NAT_MAPPER- disable NAT port mapping entirelyNB_DISABLE_PCP_HEALTH_CHECK- disable PCP health monitoringNew logs:
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Improvements
Tests